fix(async-queuer): use nullish check in #tick to handle falsy queue items - #201
fix(async-queuer): use nullish check in #tick to handle falsy queue items#201simonmeyerrr wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThis pull request fixes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/pacer/tests/async-queuer.test.ts (1)
1099-1163: Good targeted coverage for the fix.All three JS falsy primitives (
0,'',false) are exercised with mixed orderings, and both processed order andsuccessCountare asserted — this locks in the regression well.Optional: consider also adding
nullandNaNcases for completeness (both are falsy;null !== undefinedso they should also now pass through correctly). Not required for this patch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pacer/tests/async-queuer.test.ts` around lines 1099 - 1163, Add tests to cover the remaining falsy values null and NaN: create new it blocks that instantiate AsyncQueuer<T> (use AsyncQueuer<any> for NaN/null), push null and NaN items via addItem in mixed order with truthy values, call start() and advance timers with vi.advanceTimersByTimeAsync, then assert the processed array contains the original values in order and asyncQueuer.store.state.successCount equals the number of items; reference AsyncQueuer, addItem, start, store.state.successCount and vi.advanceTimersByTimeAsync when locating where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/pacer/tests/async-queuer.test.ts`:
- Around line 1099-1163: Add tests to cover the remaining falsy values null and
NaN: create new it blocks that instantiate AsyncQueuer<T> (use AsyncQueuer<any>
for NaN/null), push null and NaN items via addItem in mixed order with truthy
values, call start() and advance timers with vi.advanceTimersByTimeAsync, then
assert the processed array contains the original values in order and
asyncQueuer.store.state.successCount equals the number of items; reference
AsyncQueuer, addItem, start, store.state.successCount and
vi.advanceTimersByTimeAsync when locating where to add these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4978d571-e33c-4562-a615-b2f695d26dcf
📒 Files selected for processing (3)
.changeset/young-planes-clap.mdpackages/pacer/src/async-queuer.tspackages/pacer/tests/async-queuer.test.ts
There was a problem hiding this comment.
Pull request overview
Fixes TanStack/pacer issue #200 by ensuring AsyncQueuer#tick() doesn’t treat falsy queued items as “no item”, so values like 0, "", and false are processed instead of silently skipped.
Changes:
- Changed
#tick()’s queue-empty check from a falsy check to an explicitundefinedsentinel check. - Added tests covering processing of falsy items (
0,"",false). - Added a changeset for a patch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/pacer/src/async-queuer.ts | Updates #tick() to stop only when the next item is undefined, not when it’s merely falsy. |
| packages/pacer/tests/async-queuer.test.ts | Adds regression tests ensuring falsy items are processed. |
| .changeset/young-planes-clap.md | Declares a patch release for the falsy-item handling fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Skipping |
|
gave you co-auther credit in a larger PR that fixed this and even more issues at all once |
🎯 Changes
Fixes #200
AsyncQueuer#tick()usedif (!nextItem)to check whether the queue was empty, which caused falsy items (0,"",false) to be silently skipped. Changed toif (nextItem === undefined)to match the sentinel already used bygetNextItem()andexecute().Added tests verifying that items with values
0,"", andfalseare processed correctly.The issue is actually deeper than this, but this PR aim to make the minimal fix for this cases. A better fix would be to refactor some methods signature from
TValue | undefinedto something like{ hasItem: false } | { hasItem: true, item: TValue }in order to also allowundefinedas a value (as nothing inaddItemorTValueprevent having an undefined item), but it would require a major version change.Note: I didn't add a test for
null, because this value makes other part of the code crash (priority computing) which should be fixed in other issues/PRs.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
0, empty string,false) were incorrectly excluded from processing, ensuring they are now properly handled and processed as intended.Tests